Skip to content

feat(diagnostics): Add frontend for thread dumps#1647

Merged
andrewazores merged 22 commits intocryostatio:mainfrom
Josh-Matsuoka:thread-dumps
Aug 28, 2025
Merged

feat(diagnostics): Add frontend for thread dumps#1647
andrewazores merged 22 commits intocryostatio:mainfrom
Josh-Matsuoka:thread-dumps

Conversation

@Josh-Matsuoka
Copy link
Copy Markdown
Contributor

@Josh-Matsuoka Josh-Matsuoka commented May 30, 2025

Related to cryostatio/cryostat#135

Adds an initial frontend for triggering and displaying thread dumps in cryostat. This is done via a new button on the Diagnostics card which changes to a redirect to the diagnostics page once complete, as well as a new diagnostics page with a table for displaying and downloading captured thread dumps.

To test,

  • check out and build the agent PR (feat(diagnostics): Add thread dump capturing to diagnostics endpoint cryostat-agent#639)
  • Build the quarkus-agent test application with the above version of the cryostat agent
    -- TAGS=latest ARCHS=amd64 CRYOSTAT_AGENT_VERSION=0.6.0-SNAPSHOT ./build.bash
  • check out and build the main cryostat PR (feat(diagnostics): Add thread dumps endpoint and storage cryostat#894)
  • deploy everything via smoketest
  • Open up the diagnostics card for a target with the agent enabled (cryostat-quarkus-agent)
    -- Click the thread dump button ("Trigger thread dump"), verify that the thread dumps api is called and a notification is received indicating the thread dump completed
    -- Verify that the button changes to redirect you to the diagnostics page
  • Go to the diagnostics page, verify that the previously triggered thread dump is available in the table
    -- Verify that the last modifed time is correct and that the thread dump can be downloaded/deleted
    -- Click the upload button at the top of the table, verify that it triggers a thread dump and it appears in the table
    -- Verify that triggering new thread dumps or deleting them from the table refreshes it.

@mergify mergify bot added the safe-to-test label May 30, 2025
@Josh-Matsuoka Josh-Matsuoka added the feat New feature or request label Jun 2, 2025
@Josh-Matsuoka Josh-Matsuoka marked this pull request as ready for review June 2, 2025 20:56
@andrewazores
Copy link
Copy Markdown
Member

Review notes from recent call:

  1. needs rebase for new nav groups
  2. src/app/Shared/Services/api.utils.ts needs an update for the WebSocket notification category handling

@andrewazores
Copy link
Copy Markdown
Member

Also, some tests would be nice - at least some snapshot tests to make sure the table renders as expected when the mock ApiService returns either an empty or non-empty result.

@andrewazores
Copy link
Copy Markdown
Member

Still failing prettier checks ^

Copy link
Copy Markdown
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need something equivalent to the "All Targets Archives" view. As it stands, this UI design only allows the user to view/download/delete thread dumps which belong to a currently discovered target. If that target disappears then there is no more way via the UI to ever see those thread dumps to download or delete them.

@Josh-Matsuoka
Copy link
Copy Markdown
Contributor Author

CI doesn't seem to like toBeCalledWith, it's passing locally for me and the usage is consistent with other uses of it elsewhere so I'm not entirely sure what's wrong

@andrewazores
Copy link
Copy Markdown
Member

image image image

Could you reorganize the kebab menu for the Thread Dump items? I think the established pattern of "<other actions>, separator, delete" has been working well.

Also, when downloading a thread dump file, it currently just gets saved with the (UU)ID as the filename. I see that you have some logic in Api.service's downloadThreadDump that tries to set the filename like {target.alias}_{thread_dump.id}.thread_dump, but it seems not to work for me. You might try setting the Content-Disposition response header on the backend to get the browser to behave here.

@Josh-Matsuoka
Copy link
Copy Markdown
Contributor Author

Also, when downloading a thread dump file, it currently just gets saved with the (UU)ID as the filename. I see that you have some logic in Api.service's downloadThreadDump that tries to set the filename like {target.alias}_{thread_dump.id}.thread_dump, but it seems not to work for me. You might try setting the Content-Disposition response header on the backend to get the browser to behave here.

The content-disposition header is already set in the backend, it follows the same structure as the archived recordings download.

Looking into this further the filename is set correctly in the frontend, anchor.download gets set to it, however looking at the request that gets sent to the download endpoint and it's headers, the filename doesn't seem to be specified anywhere.

The query parameter that would set the filename is thus null when it makes it to the backend:

cryostat-1 | 2025-08-01 17:12:59,901 WARN [io.cry.dia.Diagnostics] (executor-thread-8) Handling download Request for key: (IX6TpNMZWQSweVAWEtKTrm_j0OtjGZhC_EoJv-d-o6Y=,5e42ed2f-7ca3-4f33-b3ee-60348ef45060)
cryostat-1 | 2025-08-01 17:12:59,901 WARN [io.cry.dia.Diagnostics] (executor-thread-8) Handling download Request for query: null

So it defaults to using the thread dump id as the filename.

@Josh-Matsuoka
Copy link
Copy Markdown
Contributor Author

@andrewazores any idea what's causing the above? ^

@andrewazores
Copy link
Copy Markdown
Member

Looking into this further the filename is set correctly in the frontend, anchor.download gets set to it, however looking at the request that gets sent to the download endpoint and it's headers, the filename doesn't seem to be specified anywhere.

https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/a#download

Screenshot_2025-08-28_09-13-04

The backend sets the Content-Disposition header based on the S3 filename that it knows about, which is just the UUID. The browser uses this to set the downloaded filename.

I guess that the frontend Api.service downloadFile function should set not only the anchor.download attribute, but also anchor.filename as described by that document - but the backend also sends Content-Disposition with a filename attribute, so the backend (at least in this case) will also need to correctly set the desired filename.

@Josh-Matsuoka
Copy link
Copy Markdown
Contributor Author

Download filename is now set with cryostatio/cryostat@19b5025

@andrewazores
Copy link
Copy Markdown
Member

If I open my browser's devtools and watch the network traffic when clicking on the Dashboard card item to trigger a thread dump, I see 1 request to perform the dump and then 3 identical requests in rapid succession to query available dumps (GET https://localhost:8443/api/beta/diagnostics/targets/2/threaddump). The response to the first query does not contain the new dump, and the second two responses do. It seems like only the final query should actually be sent.

Also, if I click the "Invoke Garbage Collection" button on the card instead, then there are also two identical threaddump GET queries sent.

I suspect something is not quite right with some React hooks on the Diagnostics dashboard card, causing it to re-render and re-query state too often.

@andrewazores andrewazores merged commit 84937dc into cryostatio:main Aug 28, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants